Insert parentheses around binary operation with attribute #142476
Conversation
dtolnay
added
A-pretty
|
|
rustbot
added
the
S-waiting-on-review
|
r? compiler |
|
☔ The latest upstream changes (presumably #142550) made this pull request unmergeable. Please resolve the merge conflicts. |
| // We must pretty-print `#[attr] (1 + 1)` not `#[attr] 1 + 1`. | ||
| !attrs.is_empty() | ||
| && matches!( | ||
| expr.kind, |
There was a problem hiding this comment.
The explanation you've given in https://github.com/rust-lang/rust/pull/134661/files#r2143009318 makes perfect sense, thanks!
| !attrs.is_empty() | ||
| && matches!( | ||
| expr.kind, | ||
| ast::ExprKind::Binary(..) |
There was a problem hiding this comment.
It makes me slightly uneasy that we have to "hard code" a collection of expr kinds separately here but rn I don't see a more robust alternative that's only based on (effective) precedence levels.
| ast::ExprKind::Binary(..) | ||
| | ast::ExprKind::Cast(..) | ||
| | ast::ExprKind::Assign(..) | ||
| | ast::ExprKind::AssignOp(..) |
There was a problem hiding this comment.
This assortment of expr kinds is lacking ast::Expr::Range. Consider:
macro_rules! attach { ($e:expr) => { #[allow()] $e }
fn main() { _ = attach!(0..1); }Current output is #[allow()] 0..1 which actually means (#[allow()] 0)..1. So it should be printed like so instead: #[allow()] (0..1).
As for ranges with no lower bound ..expr, ..=expr and .., the same thing applies since apparently, #[attr] ..expr (etc.) is syntactically invalid and requires parens #[attr] (..expr) (etc.).
|
Thanks a lot! r=me with |
fmease
added
S-waiting-on-author
| // If the attribute were not present on the binary operation, it would be | ||
| // legal to render this without not just the inner parentheses, but also the | ||
| // outer ones. `return x + .. .field` (Yes, really.) Currently the | ||
| // pretty-printer does not take advantage of this edge case. |
There was a problem hiding this comment.
That makes total sense and it's still insane!
There was a problem hiding this comment.
Fun fact, syn's ToTokens impls as well as prettyplease both implement this edge case, and will print with the minimal inserted parentheses, producing return x + .. .field in this case.
This test currently fails (as expected).
--- stderr -------------------------------
Pretty-printer lost necessary parentheses
BEFORE: #[attr] (1 + 1)
AFTER: #[attr] 1 + 1
Pretty-printer lost necessary parentheses
BEFORE: #[attr] (1 as T)
AFTER: #[attr] 1 as T
Pretty-printer lost necessary parentheses
BEFORE: #[attr] (x = 1)
AFTER: #[attr] x = 1
Pretty-printer lost necessary parentheses
BEFORE: #[attr] (x += 1)
AFTER: #[attr] x += 1
------------------------------------------
|
@bors r=fmease |
bors
added
S-waiting-on-bors
Rollup of 8 pull requests Successful merges: - #142384 (Bringing `rustc_rayon_core` in tree as `rustc_thread_pool`) - #142476 (Insert parentheses around binary operation with attribute) - #142485 (Marks ADT live if it appears in pattern) - #142571 (Reason about borrowed classes in CopyProp.) - #142677 (Add CI check to ensure that rustdoc JSON `FORMAT_VERSION` is correctly updated) - #142716 (Adjust `with_generic_param_rib`.) - #142756 (Make `Clone` a `const_trait`) - #142765 (rustc_target: document public AbiMap-related fn and variants) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #142476 - dtolnay:attrbinop, r=fmease Insert parentheses around binary operation with attribute Fixes the bug found by `@fmease` in #134661 (review). Previously, `-Zunpretty=expanded` would expand this program as follows: ```rust #![feature(stmt_expr_attributes)] #![allow(unused_attributes)] macro_rules! group { ($e:expr) => { $e }; } macro_rules! extra { ($e:expr) => { #[allow()] $e }; } fn main() { let _ = #[allow()] 1 + 1; let _ = group!(#[allow()] 1) + 1; let _ = 1 + group!(#[allow()] 1); let _ = extra!({ 0 }) + 1; let _ = extra!({ 0 } + 1); } ``` ```console let _ = #[allow()] 1 + 1; let _ = #[allow()] 1 + 1; let _ = 1 + #[allow()] 1; let _ = #[allow()] { 0 } + 1; let _ = #[allow()] { 0 } + 1; ``` The first 4 statements are the correct expansion, but the last one is not. The attribute is supposed to apply to the entire binary operation, not only to the left operand. After this PR, the 5th statement will expand to: ```console let _ = #[allow()] ({ 0 } + 1); ``` In the future, as some subset of `stmt_expr_attributes` approaches stabilization, it is possible that we will need to do parenthesization for a number of additional cases depending on the outcome of #127436. But for now, at least this PR makes the pretty-printer align with the current behavior of the parser. r? fmease
Fixes the bug found by @fmease in #134661 (review).
Previously,
-Zunpretty=expandedwould expand this program as follows:The first 4 statements are the correct expansion, but the last one is not. The attribute is supposed to apply to the entire binary operation, not only to the left operand.
After this PR, the 5th statement will expand to:
let _ = #[allow()] ({ 0 } + 1);In the future, as some subset of
stmt_expr_attributesapproaches stabilization, it is possible that we will need to do parenthesization for a number of additional cases depending on the outcome of #127436. But for now, at least this PR makes the pretty-printer align with the current behavior of the parser.r? fmease